Skip to content

fix(obslist): post-merge cleanup from PR #394 review (issues #2, #4)#485

Merged
brickbots merged 2 commits into
mainfrom
fix/obslist-postmerge-cleanup
Jun 20, 2026
Merged

fix(obslist): post-merge cleanup from PR #394 review (issues #2, #4)#485
brickbots merged 2 commits into
mainfrom
fix/obslist-postmerge-cleanup

Conversation

@brickbots

Copy link
Copy Markdown
Owner

Two deferred follow-up fixes from the review of #394
(feat: multi-format observing list import with native .pifinder support).
Both are low-risk and independent; neither was a merge blocker.

#2 — Cross-catalog description query comment (ui/object_details.py)

The shared ObjectsDatabase handle was documented as "only used when
cross-catalog descriptions are enabled"
— but no such setting exists and
_other_catalog_descriptions() runs unconditionally on the description view.

Resolution: keep the feature always-on and fix the comment (the product
call). The per-object query is a single indexed lookup and returns {} for
virtual planets/comets/OBS coordinate objects, so the cost is negligible.
The comment now also documents the connection lifecycle: the read handle lives
for the UI process and is released at process exit, matching the existing
per-instance ObservationsDatabase in the same file. No behaviour change.

#4 — read_list resolution loop could crash the UI (obslist.py)

read_list only wrapped the file read in try/except; the resolution loop was
unprotected, so a DB error, uninitialised catalogs, or a malformed entry
propagated to UIObsList.key_right (no handler) and crashed the UI instead of
showing the existing "Error loading" message.

  • Per-entry guard: one malformed entry is logged and skipped; the rest of
    the list still loads (result stays "success", matched < parsed).
  • Systemic failure (every entry raises, e.g. catalog DB unavailable) is
    returned as the error dict instead of a silent "0 objects" success.
  • Outer safety net mirrors the file-read error-dict contract for any
    failure outside the per-entry guard, counting len(list_catalog) so it
    can't re-raise on the same broken entries.

Return shape is unchanged, so UIObsList.key_right works without modification.

Tests

Adds TestReadListErrorHandling to tests/test_obslist_resolve.py (unit
marker) covering all three paths: per-entry skip, systemic error, and the
outer net.

pytest -m unit tests/test_obslist_resolve.py tests/test_obslist_formats.py \
               tests/test_list_descriptions.py
# 54 passed

ruff check (with builtins=['_']) and ruff format --check clean on all
changed files.

Out of scope (documented in #394's review thread): #1 real-file fixtures,
#3 session-wide description accumulation (ADR-0011), and nits #5/#6.

🤖 Generated with Claude Code

brickbots and others added 2 commits June 20, 2026 11:12
The shared ObjectsDatabase handle in object_details.py was documented as
"only used when cross-catalog descriptions are enabled" -- but there is no
such setting and _other_catalog_descriptions() always runs on the description
view. Rewrite the comment to state that it always runs and to document the
connection lifecycle (lives for the UI process, like the per-instance
ObservationsDatabase, released at process exit).

No behaviour change; the feature stays always-on (per-object query is a single
indexed lookup, skipped entirely for virtual planets/comets/OBS objects).

Post-merge cleanup from the review of PR #394 (issue #2).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ash the UI

read_list only wrapped the file read in try/except; the resolution loop
(resolve_object / _build_name_index / resolve_by_name / _coordinate_object)
was unprotected. A DB error, uninitialised catalogs, or a malformed entry
propagated to UIObsList.key_right, which has no handler -- crashing the UI
instead of showing the existing "Error loading" message.

- Per-entry guard: one malformed entry is logged and skipped, and the rest of
  the list still loads (result stays "success", matched < parsed).
- Systemic failure (every entry raises, e.g. catalog DB unavailable) is
  reported as the error dict instead of a silent "0 objects" success.
- Outer safety net mirrors the file-read error-dict contract for any failure
  outside the per-entry guard; it counts len(list_catalog) so it can't
  re-raise on the same broken `entries`.

Return shape is unchanged, so UIObsList.key_right works without modification.
Adds unit tests for all three paths (per-entry skip, systemic error, outer net).

Post-merge cleanup from the review of PR #394 (issue #4).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@brickbots brickbots merged commit bbf26bb into main Jun 20, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant